-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add otel metrics for spiderpool #81
Conversation
a022723
to
96467b1
Compare
pkg/metrics/metrics_test.go
Outdated
metricController.RegisterMetric("Node_IP_Allocation_Counts", "The total counts of node IP allocations", metrics.COUNTER) | ||
metricController.RegisterMetric("Node_IP_Allocation_Duration", "The duration of node IP allocations", metrics.HISTOGRAM) | ||
metricController.SetMetricAlert("Node_IP_Allocation_Counts", int64(1), int64(1000)) | ||
metricController.SetMetricValue("Node_IP_Allocation_Counts", int64(3), map[string]string{"type": "total", "node": "node1"}) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
// TODO: Should we return the data encoded in JSON? | ||
// GetMetricDatabase returns the current metricController metricInstance. | ||
func (mc *metricController) GetMetricDatabase() map[string]*Metric { | ||
return mc.metricInstance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
此处返回database是否需要执行json.marshal,最后返回一个string字符串?
因为直接打印这个map[string]*Metric,这里有指针,而打印并不会去解析指针的值,而是直接输出地址。
pkg/metrics/metrics.go
Outdated
} | ||
|
||
// NewMetricController will create a MetricController instance. | ||
func NewMetricController(meterName string) (MetricController, func(http.ResponseWriter, *http.Request), error) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
case HISTOGRAM: | ||
m.Metric = metric.Must(mc.meter).NewFloat64Histogram(metricName, metric.WithDescription(description)) | ||
} | ||
mc.metricInstance[metricName] = m |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about a metricName already exists ?
pkg/metrics/metrics.go
Outdated
} | ||
|
||
// RegisterMetric registers metrics. | ||
func (mc *metricController) RegisterMetric(metricName, description string, metricType MetricType) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
mi.AlertLimitUp = limitUp | ||
mi.AlertLimitDown = limitDown | ||
} else { | ||
fmt.Printf("%s does not exist", metricName) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
func (mc *metricController) SetMetricAlert(metricName string, limitUp, limitDown interface{}) { | ||
if mi, ok := mc.metricInstance[metricName]; ok { | ||
mi.AlertLimitUp = limitUp | ||
mi.AlertLimitDown = limitDown |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
} | ||
|
||
// RecordTimeMetric returns a function which will record the duration after being called. | ||
func (mc *metricController) RecordTimeMetric(metricName string, attrList map[string]string) (fn func()) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
|
||
switch metricType { | ||
case COUNTER: | ||
m.Metric = metric.Must(mc.meter).NewInt64Counter(metricName, metric.WithDescription(description)) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
return | ||
} | ||
|
||
fmt.Printf("%s does not exist", metricName) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
if m, ok := mc.metricInstance[metricName]; ok { | ||
if m.Mtype != HISTOGRAM { | ||
fmt.Printf("%s is not %v type to record duration!", metricName, HISTOGRAM) | ||
return |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Expect(err).NotTo(HaveOccurred()) | ||
}) | ||
|
||
It("use prometheus as exporter", func() { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
每个函数的健壮性不好,没有入参校验,错误了没有抛给外部 |
metric 部分的设计,是否应该做成不对业务影响的模块,不耦合?对此,我只在新建接口时才有过一次err异常抛出。 |
processor "go.opentelemetry.io/otel/sdk/metric/processor/basic" | ||
selector "go.opentelemetry.io/otel/sdk/metric/selector/simple" | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be a default logger to use
var loger = log.Stdloger.(" metric")
pkg/metrics/metrics_test.go
Outdated
metricController, httpHandle, err = metrics.NewMetricController(SpiderPoolMeter) | ||
Expect(err).NotTo(HaveOccurred()) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is easy to use DescribeTable to cover more test case
不让外部感知 erro 不叫 “不耦合”,外部不要 call function call 才叫 不耦合 |
96467b1
to
32161fb
Compare
pkg/metrics/metrics.go
Outdated
if len(metricName) == 0 { | ||
return metric.Int64Counter{}, fmt.Errorf("Failed to create metric Int64Counter, metric name is asked to be set.") | ||
} | ||
return metric.Must(meter).NewInt64Counter(metricName, metric.WithDescription(description)), nil |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
pkg/metrics/metrics.go
Outdated
if len(metricName) == 0 { | ||
return metric.Float64Histogram{}, fmt.Errorf("Failed to create metric Float64Histogram, metric name is asked to be set.") | ||
} | ||
return metric.Must(meter).NewFloat64Histogram(metricName, metric.WithDescription(description)), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if description is empty, metric.WithDescription() will go wrong ?
pkg/metrics/metrics.go
Outdated
if len(metricName) == 0 { | ||
return metric.Float64Histogram{}, fmt.Errorf("Failed to create metric Float64Histogram, metric name is asked to be set.") | ||
} | ||
return metric.Must(meter).NewFloat64Histogram(metricName, metric.WithDescription(description)), nil |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
32161fb
to
e9ee503
Compare
if len(metricName) == 0 { | ||
return nil, fmt.Errorf("Failed to create metric Int64Counter, metric name is asked to be set.") | ||
} | ||
return meter.SyncInt64().Counter(metricName, instrument.WithDescription(description)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已规避并发问题。syncInt64采用CAS自旋方式来解决并发问题。
pkg/metrics/metrics.go
Outdated
if len(metricName) == 0 { | ||
return metric.Int64Counter{}, fmt.Errorf("Failed to create metric Int64Counter, metric name is asked to be set.") | ||
} | ||
return metric.Must(meter).NewInt64Counter(metricName, metric.WithDescription(description)), nil |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
||
close(c) | ||
//Consistently(c, "60s").Should(BeClosed()) | ||
Eventually(c, "20s").Should(BeClosed()) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
pkg/metrics/metrics_test.go
Outdated
http.HandleFunc("/metrics", httpHandle) | ||
go func() { | ||
_ = http.ListenAndServe(":2222", nil) | ||
}() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
第一,这个 http server 启动后,在 it 中没有 进行 client 消费,没有起到真正的 作用,未能验证 metric真正的能输出正确的值
第二,,在 整工程 跑单元测试时,这个 协程泄露,没有回收
DeferCleanup(func() {
close http server
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议 if server.ListenAndServe() { printf " I am existing , info=%v ". %v} , 万一 本地端口本占用呢?或者 中途 挂了,或者 知道 afterall 通知推出了
SpiderPoolMeter = "spider_pool_meter" | ||
MetricNodeIPAllocationCountsName = "Node_IP_Allocation_Counts" | ||
MetricNodeIPAllocationDurationName = "Node_IP_Allocation_Duration" | ||
) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
pkg/metrics/metrics_test.go
Outdated
MetricNodeIPAllocationDuration syncfloat64.Histogram | ||
} | ||
|
||
var _ = Describe("metrics", Label("unitest"), Ordered, func() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
e9ee503
to
0fbeb2b
Compare
Codecov Report
@@ Coverage Diff @@
## main #81 +/- ##
==========================================
+ Coverage 77.58% 81.52% +3.94%
==========================================
Files 2 3 +1
Lines 116 157 +41
==========================================
+ Hits 90 128 +38
- Misses 19 21 +2
- Partials 7 8 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
对于metrics的unitest记录某种指标是否成功记录/消费的验证,otel库目前缺少该功能。已有其他contributor提过该类的issue和pr,但是还没有merge。 |
94e34bb
to
a64f45e
Compare
有冲突了,解决下 |
Signed-off-by: Icarus9913 <icaruswu66@qq.com>
a64f45e
to
2179714
Compare
1dbeba8
to
c41344e
Compare
pkg/metrics/metrics_suite_test.go
Outdated
|
||
err = server.Serve(listener) | ||
if nil != err && err != http.ErrServerClosed && err != net.ErrClosed { | ||
logger.Error(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ginkgo 用的是 BY() 或 FAIL() 或 GinkgoWriter.Printf , 这种日志才能追加到 测试报告中,
没有 使用 logger 或者 printf 的,单元测试成千上万的 日志,是没法看 的,也不能在测试报告中看到
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
解决下
} | ||
|
||
// verify counts instrument | ||
if strings.Contains(line, MetricNodeIPAllocationCountsName) && strings.Contains(line, OTEL_COUNTS_SIGNAL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如果是能 把 json字符串 解析成 struct, 使用 struct 成员 来 值判断,会是更加 正道的 方法
尤其是 一些 validator 库的使用 ,能加速效率
https://github.com/go-playground/validator/blob/master/_examples/simple/main.go
https://github.com/gookit/validate/blob/master/README.zh-CN.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prometheus handler对于http.ResponseWriter,是采用text文本返回。
https://github.com/prometheus/common/blob/627089d3a7af73be778847aa577192b937b8d89a/expfmt/encode.go#L86
https://github.com/prometheus/common/blob/627089d3a7af73be778847aa577192b937b8d89a/expfmt/text_create.go#L67
Signed-off-by: Icarus9913 <icaruswu66@qq.com>
c41344e
to
3a7b9ad
Compare
# Conflicts: # go.mod # go.sum # vendor/modules.txt
Signed-off-by: Icarus9913 <icaruswu66@qq.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
This pull request has been automatically marked as stale because it |
Signed-off-by: Icarus9913 icaruswu66@qq.com
Add otel metrics module for spiderpool to monitor.